[WRONG BRANCH] test(ws-upstream): avoid unsupported fake timer advance - #267
[WRONG BRANCH] test(ws-upstream): avoid unsupported fake timer advance#267luvs01 wants to merge 1 commit into
Conversation
|
✅ Deterministic PR hygiene checks passed. |
📝 WalkthroughWalkthroughThe WebSocket upstream timeout test replaces Jest fake timers with a Bun ChangesWebSocket timeout test
Estimated code review effort: 1 (Trivial) | ~5 minutes Mergeability Score: 🔵 Low · up to The test-only change may allow the upgrade-deadline scenario to pass without confirming that the timeout callback was installed, leaving the 10-second fallback behavior insufficiently verified. The PR is mergeable with explicit owner follow-up to assert the callback was captured. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
⏳ DRAFT
What to do
Its title has been prefixed with |
github-actions
Bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/ws-upstream.test.ts`:
- Around line 210-217: Update the test around the deadline invocation to assert
that deadline was captured before calling it, ensuring the test fails when
setTimeout does not register the fallback callback. Keep the existing response,
fallbackCalls, and socket-closed assertions unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 028b1c28-74af-4fb5-9b62-4b1747bd18f6
📒 Files selected for processing (1)
tests/ws-upstream.test.ts
| deadline?.(); | ||
| const response = await responsePromise; | ||
|
|
||
| expect(response).toBe(sentinel); | ||
| expect(fallbackCalls).toBe(1); | ||
| expect(FakeWebSocket.instances[0].closed).toBe(true); | ||
| } finally { | ||
| jest.useRealTimers(); | ||
| setTimeoutSpy.mockRestore(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that the timeout callback was captured.
In tests/ws-upstream.test.ts, Line 210 skips the callback when deadline is undefined. The assertion at Line 195 does not detect a missing timer because it runs only when setTimeout is called. The test can then hang or complete through a different close path without testing the 10-second fallback.
Assert the callback before invoking it.
Proposed fix
- deadline?.();
+ expect(deadline).toBeDefined();
+ deadline!();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| deadline?.(); | |
| const response = await responsePromise; | |
| expect(response).toBe(sentinel); | |
| expect(fallbackCalls).toBe(1); | |
| expect(FakeWebSocket.instances[0].closed).toBe(true); | |
| } finally { | |
| jest.useRealTimers(); | |
| setTimeoutSpy.mockRestore(); | |
| expect(deadline).toBeDefined(); | |
| deadline!(); | |
| const response = await responsePromise; | |
| expect(response).toBe(sentinel); | |
| expect(fallbackCalls).toBe(1); | |
| expect(FakeWebSocket.instances[0].closed).toBe(true); | |
| } finally { | |
| setTimeoutSpy.mockRestore(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/ws-upstream.test.ts` around lines 210 - 217, Update the test around the
deadline invocation to assert that deadline was captured before calling it,
ensuring the test fails when setTimeout does not register the fallback callback.
Keep the existing response, fallbackCalls, and socket-closed assertions
unchanged.
1 participant
Motivation
jest.advanceTimersByTime, which is not exported by thejesthelper in the Bun test runtime and caused a TypeError in CI.bun:testenvironment.Description
jestimport withspyOnfrombun:testand remove calls tojest.useFakeTimers()/jest.advanceTimersByTimein the failing test.globalThis.setTimeoutthat captures the upgrade-deadline callback, asserts the expected10_000delay, invokes the callback to simulate the deadline, and then restores the spy infinallyto avoid test pollution.Testing
bun test tests/ws-upstream.test.ts -t 'upgrade deadline elapses'and it passed.bun run typecheckandbun run privacy:scan, both completed successfully.bun run test) during verification; the modified ws-upstream test passed, but unrelated existing failures and external GUI build/network issues prevented a fully green full-suite run in this environment.Codex Task
Summary by CodeRabbit